feat(shim-opentracing): update logging based on new spec #2282
Conversation
* use event name when available * map error attributes to semantic conventions * use provided timestamp * fix shim example: runtime error and missing service.name
vreynolds
requested review from
dyladan,
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
MSNev,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
| const entries = Object.entries(attributes); | ||
| const errorEntry = entries.find(([key]) => key === 'error.object'); | ||
| const error = errorEntry?.[1]; | ||
| if (typeof error === "string") { |
There was a problem hiding this comment.
string is the only "exception" type that's possible here, since the override method takes SpanAttributes, meaning shim consumers can't log an attribute like error.object: new Error(), if they're using type safety
There was a problem hiding this comment.
do you mean the error type from our api or from opentracing conventions ? Because i believe we handle error object too ?
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-tracing/src/Span.ts#L189
https://github.com/open-telemetry/opentelemetry-js-api/blob/41109c83a9784d689f319f2c5d953b3874c694a3/src/common/Exception.ts#L43
There was a problem hiding this comment.
You are correct, recordException can take an Error object, but the log function itself takes SpanAttributes which don't allow objects as attribute values:
The issue here is that the shim restricts the types more than the OpenTracing lib: https://github.com/opentracing/opentracing-javascript/blob/111ea4f7939c8f8f538333330d72115e5b28bcce/src/span.ts#L143 where you can pass any as attribute values, including objects.
Codecov Report
@@ Coverage Diff @@
## main #2282 +/- ##
==========================================
+ Coverage 92.74% 92.79% +0.04%
==========================================
Files 145 145
Lines 5184 5215 +31
Branches 1060 1068 +8
==========================================
+ Hits 4808 4839 +31
Misses 376 376
|
|
Looks like new node 16 build is unhappy with something in the NPM cache... I'm seeing the same error across other PRs that merged |
|
re-running with no cache only helps for the first run, then after that it's broken again. i was looking into it this morning. looks like we'll have to add a chown or chmod to the ci action or something |
examples/opentracing-shim/shim.js
Outdated
|
|
||
| provider.addSpanProcessor(new SimpleSpanProcessor(getExporter(serviceName))); | ||
| // Initialize the OpenTelemetry APIs to use the NodeTracerProvider bindings | ||
| provider.register(); | ||
|
|
||
| registerInstrumentations({ | ||
| }); | ||
| registerInstrumentations({}); |
There was a problem hiding this comment.
this will no longer register any default instrumentations so it can be removed or some default instrumentations should be added here
There was a problem hiding this comment.
Updated! Removed the line, since server.js and client.js generate spans inline.
Which problem is this PR solving?
Short description of the changes